-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add more metrics for reindex #137597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add more metrics for reindex #137597
Conversation
...dex/src/internalClusterTest/java/org/elasticsearch/index/reindex/ReindexPluginMetricsIT.java
Show resolved
Hide resolved
|
Pinging @elastic/es-data-management (Team:Data Management) |
PeteGillinElastic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @samxbr . I haven't done a full review on this, since you said you were looking for early feedback, but here are some initial thoughts.
We also talked about attempting to get lost operations (due to node restart). Did you look at how the existing chart for that works? Is it grepping the logs? Do you know where that logging is done? I'm wondering whether we want to try to figure out how to distinguish remote vs local in there, too.
...dex/src/internalClusterTest/java/org/elasticsearch/index/reindex/ReindexPluginMetricsIT.java
Show resolved
Hide resolved
modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java
Outdated
Show resolved
Hide resolved
Good question, I assume you are referring to the {
"match_phrase": {
"log.logger": "org.elasticsearch.node.ShutdownPrepareService"
}
},
{
"match_phrase": {
"log.level": "WARN"
}
},
{
"match_phrase": {
"message": "*reindex*"
}
}We probably need to implement something else to capture remote/local there, I can take a deeper look on that later. |
Yes, that's what I was referring to. Thanks. |
Looking at the code, I don't think it's going to be straightforward to get the remote/local info in there. The code you linked is generic task-related code, and we are only identifying reindex tasks via a regex on the task name. I assume that changing the task name so that it included |
PeteGillinElastic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Sam! Just one substantive comment (and one nit).
modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java
Show resolved
Hide resolved
modules/reindex/src/main/java/org/elasticsearch/reindex/ReindexMetrics.java
Outdated
Show resolved
Hide resolved
BASE=afd3a426eabdfda7d4fd6b0c52d76162e3c9c47e HEAD=26abb9d1597bc46b560996f1854ea01e858f061f Branch=main
BASE=afd3a426eabdfda7d4fd6b0c52d76162e3c9c47e HEAD=26abb9d1597bc46b560996f1854ea01e858f061f Branch=main
Adds a new metric
es.reindex.completion.totalto track the number of completed reindex operations, along with these metric attributes to identify different results:error.type: if present, indicates the reindex failed with the specified exception. Otherwise indicates the reindex was sucessfulreindex.source:localorremote, indicates whether the source cluster was the local or a remote clusteres.reindex.duration.histogrammetric